Repository navigation
Conversation
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks, the fix is correct. Verified locally: the new test fails against the main wasm (binding 255 is dropped), the committed lib/lexer.wasm reproduces byte-identically via build-wasm-local, and the analysis memory budget holds under stress (100k minimal var a=require("a") declarations at both analysis_base alignments, since the shortest tracked declaration reserves 34 bytes against the 20-byte record).
Main suggestion is structural: with the overflow path in place, the static array is now just a fast path for the first 255 entries at the cost of two storage paths and two lookup loops. Putting all bindings in analysis memory as a single linked list ends up simpler and smaller, see inline comment.
Also worth rebasing on main (#131 landed, no overlap).
73e1781 to
0cf6148
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Verified locally: the new tests fail against the main wasm and pass in all three test modes with this change, including a stress check with 100k separator-less var a=require("") bindings at even and odd source lengths.
Reusing the analysis arena is the right call here. One non-blocking nit inline on documenting the arena bound now that two record types depend on it.
| } | ||
| starExportStack++; | ||
| StarExportBinding* binding = (StarExportBinding*)analysis_head; | ||
| analysis_head += sizeof(StarExportBinding); |
There was a problem hiding this comment.
Nit: with bindings now allocated from the arena as well, the len * 4 sizing in src/lexer.js becomes load-bearing for two record types and nothing states the rule. Suggest expanding the comment there, something like:
// 2 bytes per code point for the source, then the same again as the analysis
// arena. Every record allocated there (12-byte Slice per export/reexport,
// 20-byte StarExportBinding per `var x=require("")`) must stay under 2 bytes
// per char of its shortest producing pattern, or the arena can overflow.The binding record is the thinnest margin at 17 chars for 20 bytes.
The Wasm parser stops recording bindings when its fixed array fills, so later Babel-style reexports disappear from the result. Overflow bindings use the existing per-source analysis memory, and files within the static capacity retain the existing storage path. Signed-off-by: Ruben Bridgewater <ruben.bridgewater@datadoghq.com>
A minimal declaration reserves 34 analysis bytes for a 20-byte Wasm binding record. This lets the existing source allocation hold bindings without a separate overflow representation. Signed-off-by: Ruben Bridgewater <ruben.bridgewater@datadoghq.com>
Dense shorthand exports allocate a 12-byte record for every two source code units. The previous budget can overflow when require bindings also use the analysis arena, including inputs that parse successfully on main. Reserve six analysis bytes per UTF-16 code unit. This doubles the linear memory reservation but avoids capacity checks for each record.
0cf6148 to
5d0ce15
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
The binding change is unchanged since my approval and still looks good: rebased cleanly, the committed lib/lexer.wasm rebuilds byte-identically, and all tests pass in the three modes.
The new third commit is the len * 8 reservation from #134's first revision, which #134 has since replaced with growing on demand from sa()/_add*. I'd suggest dropping it here so the memory bound is owned by one PR. Note dense a, exports already trap at len * 4 on main with no bindings at all, so it isn't this PR's bindings that break the budget (they add at most ~1.2 bytes per code unit).
The one thing that does need coordinating is the binding allocation itself, see inline. Simplest order is probably #134 first, then rebase this on top with the capacity check added to the binding path.
| // StarExportBinding records (at least 17 code units for var a=require("")). | ||
| let memory = wasm.memory.buffer; | ||
| const extraMem = (wasm.__heap_base.value || wasm.__heap_base) + len * 4 - memory.byteLength; | ||
| const extraMem = (wasm.__heap_base.value || wasm.__heap_base) + len * 8 - memory.byteLength; |
There was a problem hiding this comment.
Suggest dropping this commit and keeping len * 4 here; #134 now grows on demand and reserves source space only, so this line conflicts with it directly.
| } | ||
| starExportStack++; | ||
| StarExportBinding* binding = (StarExportBinding*)analysis_head; | ||
| analysis_head += sizeof(StarExportBinding); |
There was a problem hiding this comment.
This bump has no capacity check, so once #134 lands (source-only reservation, growth inside _addExport etc.) this path traps. I simulated that by setting the reservation to len * 2 on this branch, and both binding stress tests fail with memory access out of bounds. Whichever of the two lands second needs an ensureAnalysisCapacity(sizeof(StarExportBinding)) before this line.
| assert.deepStrictEqual(parse(source), { exports: ['value'], reexports: ['side', 'last'] }); | ||
| }); | ||
|
|
||
| test('bounds shared analysis space for dense object exports', () => { |
There was a problem hiding this comment.
This test only detects the bug in isolation. Run alone with len * 4 it fails as intended, but in the full suite with len * 4 it passes, because the 4096-binding tests above have already grown memory and Wasm memory never shrinks. #134's test/wasm-memory.js uses fresh instances, which is where this regression belongs.
The Wasm parser stops recording require bindings after 255 declarations, so later Babel-style reexports disappear. Binding records now share per-source analysis memory and reset between parses.